refactor(api): require resource in CheckResourcePermission - #1886
Conversation
CheckResourcePermission reads the object only from the resource field
("namespace:id") and returns InvalidArgument when it is missing or
malformed. The deprecated object_id/object_namespace request fields are
no longer read. E2E tests send the resource form.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughPermission handlers now require namespace-qualified values in ChangesPermission resource identifier migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The API now requires the documented resource format and updates affected callers and tests; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a381cba-390a-4d76-b8c0-f350fcbc32c1
📒 Files selected for processing (5)
internal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.gotest/e2e/regression/api_test.gotest/e2e/regression/onboarding_test.gotest/e2e/regression/serviceusers_test.go
Coverage Report for CI Build 32704499925Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 48.897%Details
Uncovered Changes
Coverage Regressions225 previously-covered lines in 10 files lost coverage.
Coverage Stats
💛 - Coveralls |
BatchCheckPermission rejects a resource with an empty namespace or id as InvalidArgument instead of failing at SpiceDB as internal. All three resource guards in the file, and the federated subject guard, return the namespace-notation error so callers see the expected format. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49074c84-636d-440d-a48b-0457f5e47b3a
📒 Files selected for processing (2)
internal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.go
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rohilsurana
left a comment
There was a problem hiding this comment.
Review against main. The change itself is a clean tightening. A couple of behavior and test notes below.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
CheckResourcePermission accepts the object only via the
resourcefield (namespace:id). The deprecatedobject_id/object_namespacerequest fields are no longer read; requests sending only those fields now receive InvalidArgument. Namespace aliases keep working insideresource. Part of #1782.Changes
internal/api/v1beta1connect/permission_check.go: remove the split-field fallback; reject a missing or malformedresourcetest/e2e/regression/onboarding_test.go,serviceusers_test.go,api_test.go: check requests sendresourceinternal/api/v1beta1connect/permission_check_test.go: success cases sendresource; add missing-resource caseTest Plan
go test ./internal/api/v1beta1connect/passesmake lintpasses (0 issues)🤖 Generated with Claude Code